Update the format of selected state and propagate it to the treeview#1817
Update the format of selected state and propagate it to the treeview#1817
Conversation
This format will make implementation much simpler than having to parse JSON paths.
shati-patel
left a comment
shati-patel
left a comment
There was a problem hiding this comment.
LGTM! Thanks for doing this ⚡
| export function isRemoteSystemDefinedListDbItem( | ||
| dbItem: DbItem, | ||
| ): dbItem is RemoteSystemDefinedListDbItem { | ||
| return dbItem.kind === DbItemKind.RemoteSystemDefinedList; | ||
| } | ||
|
|
||
| export function isRemoteUserDefinedListDbItem( | ||
| dbItem: DbItem, | ||
| ): dbItem is RemoteUserDefinedListDbItem { | ||
| return dbItem.kind === DbItemKind.RemoteUserDefinedList; | ||
| } | ||
|
|
||
| export function isRemoteOwnerDbItem( | ||
| dbItem: DbItem, | ||
| ): dbItem is RemoteOwnerDbItem { | ||
| return dbItem.kind === DbItemKind.RemoteOwner; | ||
| } | ||
|
|
||
| export function isRemoteRepoDbItem(dbItem: DbItem): dbItem is RemoteRepoDbItem { | ||
| return dbItem.kind === DbItemKind.RemoteRepo; | ||
| } | ||
|
|
||
| export function isLocalListDbItem(dbItem: DbItem): dbItem is LocalListDbItem { | ||
| return dbItem.kind === DbItemKind.LocalList; | ||
| } | ||
|
|
||
| export function isLocalDatabaseDbItem( | ||
| dbItem: DbItem, | ||
| ): dbItem is LocalDatabaseDbItem { | ||
| return dbItem.kind === DbItemKind.LocalDatabase; | ||
| } |
There was a problem hiding this comment.
These 30 lines are only created for tests. Isn't that a huge code smell? Can't we solve the problem differently? The two isLocal... are not used at all.
There was a problem hiding this comment.
We can definitely solve this differently - it requires checking the dbItem.kind and then casting the item to the relevant type, which is a little bit more fiddly that simply using type guards.
We could move the code somewhere else too since it's just for tests but I think that would make it harder to find and much more likely for this code to get duplicated elsewhere. We could also only implement what is needed (and don't include the type guards for local dbs) but to me that feels incomplete.
And despite this being 30 LoC, it's 30 lines of straight-forward code.
So in general I think you have the right reactions seeing this code, but I think this is a case where it's ok to break the "rules" a bit. And the db-item module comes with a more complete interface.
3 participants
The format we had initially decide on was unfortunately not ideal in terms of implementation so we're changing it to be more explicit. In general we don't expect users to manually define that state in the config, apart from some exceptional cases. The configuration schema has been updated to help in those rare cases. See commit 943c547
We then read the configuration state and propagate selection all the way to the tree view items. See commit 7b54b6c. We currently don't do anything with that, but in a following PR we'll use that information to show the relevant icons and actions.
Checklist
N/A:
ready-for-doc-reviewlabel there.